packages/loot-core/src/server/api.ts (3)

254-260: Simplify error collection for individual account sync.

The error collection can be simplified by spreading the errors array directly.

-    allErrors.push(errors);
+    allErrors.push(...errors);

---

261-281: Optimize account filtering logic.

The account filtering can be optimized to avoid multiple array operations.

-    const accountIdsToSync = accountsData.map(a => a.id);
-    const simpleFinAccounts = accountsData.filter(
-      a => a.account_sync_source === 'simpleFin',
-    );
-    const simpleFinAccountIds = simpleFinAccounts.map(a => a.id);
+    const [simpleFinAccounts, otherAccounts] = accountsData.reduce(
+      ([sfAccs, others], acc) => {
+        if (acc.account_sync_source === 'simpleFin') {
+          sfAccs.push(acc);
+        } else {
+          others.push(acc.id);
+        }
+        return [sfAccs, others];
+      },
+      [[], []]
+    );
+    const simpleFinAccountIds = simpleFinAccounts.map(a => a.id);

---

283-285: Consider aggregating multiple errors for better error reporting.

Currently, only the first error is thrown, which might hide other sync issues. Consider aggregating all errors to provide a complete sync status.

-  const errors = allErrors.filter(e => e != null);
-  if (errors.length > 0) {
-    throw new Error(getBankSyncError(errors[0]));
+  const errors = allErrors.filter(e => e != null);
+  if (errors.length > 0) {
+    const errorMessages = errors.map(e => getBankSyncError(e));
+    throw new Error(
+      `Multiple sync errors occurred:\n${errorMessages.join('\n')}`
+    );

packages/loot-core/src/server/main.ts (1)

1102-1111: Improve Error Message for Better Debugging

The error message thrown when both id and ids are provided or neither is specified could be more informative. Including the received values of id and ids can aid in debugging.

Apply this diff to enhance the error message:

if ((id && ids) || (!id && !ids)) {
-  throw new Error('Specify either id or ids, but not both');
+  throw new Error(`Specify either 'id' or 'ids', but not both. Received id: ${id}, ids: ${ids}`);
}

Configuration used: CodeRabbit UI
Review profile: CHILL

Learnt from: matt-fidd
PR: actualbudget/actual#3581
File: packages/loot-core/src/client/actions/account.ts:180-194
Timestamp: 2024-11-04T00:34:13.035Z
Learning: In `packages/loot-core/src/client/actions/account.ts`, within the `syncAccounts` function, the batch sync request for SimpleFin accounts handles errors by returning error objects instead of throwing exceptions. Therefore, wrapping this block in a try-catch is unnecessary.

Learnt from: matt-fidd
PR: actualbudget/actual#3581
File: packages/loot-core/src/types/models/bank-sync.d.ts:11-21
Timestamp: 2024-11-04T14:14:10.698Z
Learning: In `packages/loot-core/src/types/models/bank-sync.d.ts`, when defining TypeScript types for data received from the server, maintain the field names as they are in the server response, even if they don't follow TypeScript naming conventions.

#!/bin/bash
# Description: Verify the usage of SimpleFin batch sync functionality

# Check for SimpleFin account configurations
echo "Checking SimpleFin account configurations..."
rg -l "account_sync_source.*simpleFin" 

# Check for batch sync handler usage
echo "Checking batch sync handler usage..."
rg -A 5 "simplefin-batch-sync"

# Check for potential migration needs
echo "Checking for old sync patterns that might need updating..."
rg -A 5 "accounts-bank-sync.*id:"

#!/bin/bash
# Let's verify the implementation more thoroughly

# Check the implementation of accounts-bank-sync handler
echo "Checking accounts-bank-sync implementation..."
ast-grep --pattern 'handlers["accounts-bank-sync"] = async function'

# Check for any other sync implementations that might need updating
echo "Checking other sync implementations..."
rg -A 10 "bank-sync" --type ts

# Check for SimpleFin related code to ensure consistency
echo "Checking SimpleFin related implementations..."
rg -A 5 "account_sync_source.*=.*simpleFin" --type ts

#!/bin/bash
# Description: Find all occurrences of 'account_sync_source' comparisons to verify consistent casing.

# Search for comparisons where 'account_sync_source' is checked against a string literal.
rg -t ts "account_sync_source\s*=\s*['\"]\w+['\"]"

#!/bin/bash
# Search for any occurrence of 'simpleFin' with different casings
rg -i "simplefin" -t ts -A 2 -B 2

packages/loot-core/src/server/main.ts (1)

1102-1122: Consider simplifying the interface to only accept an array of IDs.

As suggested in a previous review, consider simplifying this handler to only accept an optional ids array parameter. This would:

• Reduce complexity in parameter validation
• Make the interface more consistent
• Allow single account sync through an array with one ID

Example refactor:

-handlers['accounts-bank-sync'] = async function ({ id, ids }) {
+handlers['accounts-bank-sync'] = async function ({ ids = [] }) {
   const [[, userId], [, userKey]] = await asyncStorage.multiGet([
     'user-id',
     'user-key',
   ]);

-  if ((id && ids) || (!id && !ids)) {
-    throw new Error('Specify either id or ids, but not both');
-  }

   const accounts = await db.runQuery(
     `
     SELECT a.*, b.bank_id as bankId 
     FROM accounts a
     LEFT JOIN banks b ON a.bank = b.id
     WHERE a.tombstone = 0 AND a.closed = 0 
-      ${id ? 'AND a.id = ?' : ''} 
-      ${ids ? `AND a.id IN (${ids.map(() => '?').join(', ')})` : ''}
+      ${ids.length ? `AND a.id IN (${ids.map(() => '?').join(', ')})` : ''}
     ORDER BY a.offbudget, a.sort_order
   `,
-    id ? [id] : ids || [],
+    ids,
     true,
   );

Configuration used: CodeRabbit UI
Review profile: CHILL

CREATE INDEX idx_accounts_sync_source ON accounts(account_sync_source) WHERE tombstone = 0 AND closed = 0;

packages/loot-core/src/client/actions/account.ts (2)

Line range hint 164-181: Consider optimizing the account filtering logic.

The account filtering and sorting logic could be extracted into a separate utility function for better reusability and testability.

+ function getAccountsToSync(accounts, filterSimpleFin = false) {
+   const filtered = accounts.filter(
+     ({ bank, closed, tombstone }) => !!bank && !closed && !tombstone,
+   );
+   if (filterSimpleFin) {
+     filtered = filtered.filter(a => a.account_sync_source === 'simpleFin');
+   }
+   return filtered.sort((a, b) =>
+     a.offbudget === b.offbudget
+       ? a.sort_order - b.sort_order
+       : a.offbudget - b.offbudget,
+   ).map(({ id }) => id);
+ }

  let accountIdsToSync = !batchSync
    ? [id]
-   : getState()
-       .queries.accounts.filter(
-         ({ bank, closed, tombstone }) => !!bank && !closed && !tombstone,
-       )
-       .sort((a, b) =>
-         a.offbudget === b.offbudget
-           ? a.sort_order - b.sort_order
-           : a.offbudget - b.offbudget,
-       )
-       .map(({ id }) => id);
+   : getAccountsToSync(getState().queries.accounts);

---

Line range hint 182-196: Consider adding logging for debugging and monitoring.

The batch sync implementation would benefit from additional logging to help track sync progress and diagnose issues in production.

  if (batchSync && simpleFinAccounts.length > 0) {
-   console.log('Using SimpleFin batch sync');
+   console.log(`Starting SimpleFin batch sync for ${simpleFinAccounts.length} accounts`);
    const res = await send('simplefin-batch-sync', {
      ids: simpleFinAccounts.map(a => a.id),
    });
+   console.log(`SimpleFin batch sync completed with ${res.length} responses`);

packages/loot-core/src/server/main.ts (1)

1102-1117: LGTM! Consider minor formatting improvements for SQL readability.

The changes to support batch synchronization through the ids parameter are well-implemented. The SQL query is secure and efficient.

Consider reformatting the SQL query for better readability:

-    SELECT a.*, b.bank_id as bankId 
-    FROM accounts a
-    LEFT JOIN banks b ON a.bank = b.id
-    WHERE a.tombstone = 0 AND a.closed = 0
-      ${ids.length ? `AND a.id IN (${ids.map(() => '?').join(', ')})` : ''}
-    ORDER BY a.offbudget, a.sort_order
+    SELECT a.*, b.bank_id as bankId
+    FROM accounts a
+    LEFT JOIN banks b ON a.bank = b.id
+    WHERE a.tombstone = 0
+      AND a.closed = 0
+      ${ids.length ? `AND a.id IN (${ids.map(() => '?').join(', ')})` : ''}
+    ORDER BY a.offbudget, a.sort_order

Configuration used: CodeRabbit UI
Review profile: CHILL

Learnt from: matt-fidd
PR: actualbudget/actual#3581
File: packages/loot-core/src/client/actions/account.ts:180-194
Timestamp: 2024-11-10T16:45:25.627Z
Learning: In `packages/loot-core/src/client/actions/account.ts`, within the `syncAccounts` function, the batch sync request for SimpleFin accounts handles errors by returning error objects instead of throwing exceptions. Therefore, wrapping this block in a try-catch is unnecessary.

#!/bin/bash
# Description: Check for consistent error handling across sync implementations

# Look for error handling patterns in sync-related code
rg -A 5 "SyncError|markAccountFailed" 

# Check for any SimpleFin-specific error types
ast-grep --pattern 'error.type === $type'

- const errors = allErrors.filter(e => e != null);
- if (errors.length > 0) {
-   throw new Error(getBankSyncError(errors[0]));
- }
+ const errors = allErrors.filter(e => e != null);
+ if (errors.length > 0) {
+   const errorMessages = errors.map(e => getBankSyncError(e));
+   throw new Error(`Multiple sync errors occurred:\n${errorMessages.join('\n')}`);
+ }

#!/bin/bash
# Description: Check error handling patterns in sync operations
# Look for error handling patterns in sync operations
ast-grep --pattern 'catch ($err) { $$$  }'

# Check for error aggregation patterns
rg -A 5 "errors\.(push|concat)"

- const accountsData = await handlers['accounts-get']();
- const accountIdsToSync = accountsData.map(a => a.id);
- const simpleFinAccounts = accountsData.filter(
-   a => a.account_sync_source === 'simpleFin',
- );
- const simpleFinAccountIds = simpleFinAccounts.map(a => a.id);
+ const accountsData = await handlers['accounts-get']();
+ const [simpleFinAccounts, otherAccounts] = accountsData.reduce(
+   (acc, account) => {
+     acc[account.account_sync_source === 'simpleFin' ? 0 : 1].push(account);
+     return acc;
+   },
+   [[], []]
+ );
+ const simpleFinAccountIds = simpleFinAccounts.map(a => a.id);
+ const otherAccountIds = otherAccounts.map(a => a.id);